Skip to content

Conversation

@shroffk
Copy link
Contributor

@shroffk shroffk commented Jul 30, 2018

Have to wait till PR#18 is merged before this can be merged.

shroffk added 10 commits June 18, 2018 12:53
Conflicts:
	epics-vtype/vtype/src/main/java/org/epics/vtype/VType.java
Adding getter and putter for ListNumber on PVDoubleArray
Adding ListNumber support for PVIntArray and PVNumberArray superclass.
Integrated ListNumber support into signed numeric arrays.
Adding unsigned array support
Renamed XxxIntYyy to XxxIntegerYyy so it better follows Java conventions
@shroffk shroffk requested a review from georgweiss August 3, 2018 15:33
@georgweiss
Copy link
Contributor

Does not seem to build on Java 10 (several issues).

As for technical matters, I have insufficient experience with the code to say anything clever. However, I do have some comments in regards to VImage related classes:

  1. Is the VImageType enum supposed to cover all relevant permutations of properties like colour space, number of bytes per pixel and others? If so, why not use a class that encapsulates these properties?
  2. In VImgeDataType, why is there a need for a string type? Plus, the methods in the class are tightly dependent on the order in which the values appear in the source code. As an alternative, one could maybe consider an integer (or short) property in the enum in order to not depend on the ordinal, e.g. pvBoolean(0), pvByte(1), pvShort(2) etc.

@shroffk
Copy link
Contributor Author

shroffk commented Aug 6, 2018

Does not seem to build on Java 10 (several issues).

The master itself is not building on jdk10 as yet, have expanded the scope of the
PR 17 #17

Is the VImageType enum supposed to cover all relevant permutations of properties like colour space, number of bytes per pixel and others? If so, why not use a class that encapsulates these properties?

I think the list is intended to be fixed to what area detector can handle.

In VImgeDataType, why is there a need for a string type? Plus, the methods in the class are tightly dependent on the order in which the values appear in the source code. As an alternative, one could maybe consider an integer (or short) property in the enum in order to not depend on the ordinal, e.g. pvBoolean(0), pvByte(1), pvShort(2) etc.

The class was based on the org.epics.pvdata.ScalarType , I wanted to keep it as similar as possible so that client code dealing with Types would remain the same.
I will ask anyone if it makes sense to keep the pvString, else we could consider to remove it.

Now that I look at it in detail, I agree with you that the heavy use of ordinal is rather unfortunate.
Ideally I would feel that the enum should be able to tell you if the type is a primitive, an integer, a unsigned value, etc....without having to check the ordinal.
However, this class was designed to match org.epics.pvdata.ScalarType I would like to keep them the same...we should consider fixing the org.epics.pvdata.ScalarType but that would require checking up with the other developers. This can be a discussion topic for the Aug14th meeting.

@georgweiss
Copy link
Contributor

OK, I admit I have not inspected the commits in detail, but I think that we should try to spread the word that any new code should be Java 10 compliant. My impression is that Javadoc stuff that needs most attention.

@georgweiss
Copy link
Contributor

I was under the impression that my fixes for Java 9+ were merged already to master. Can you please clarify?

@shroffk
Copy link
Contributor Author

shroffk commented Aug 13, 2018

@georgweiss I too was under the impression that all your PR's have been merged.

@shroffk
Copy link
Contributor Author

shroffk commented Aug 13, 2018

@georgweiss it seems like the javadoc issues are now with https://github.com/epics-base/exampleJava I will request Marty to have a look at it

From Marty

I have been spending all my time on C++ code so I did not pay attention to this request until now.
I have checked out branch updated_pvdata and will start looking at it.

@mrkraimer
Copy link
Contributor

I am at

mrk> java -version
openjdk version "1.8.0_181"
OpenJDK Runtime Environment (build 1.8.0_181-b13)
OpenJDK 64-Bit Server VM (build 25.181-b13, mixed mode)
mrk> pwd
/home/epicsv4/masterJava/exampleJava
mrk> cat .git/config 
[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
[remote "origin"]
	url = https://github.com/epics-base/exampleJava.git
	fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
	remote = origin
	merge = refs/heads/master
mrk> 

Then

mrk> mvn package
...
Building index for all classes...
Generating /home/epicsv4/masterJava/exampleJava/serviceAPI/target/apidocs/allclasses-frame.html...
Generating /home/epicsv4/masterJava/exampleJava/serviceAPI/target/apidocs/allclasses-noframe.html...
Generating /home/epicsv4/masterJava/exampleJava/serviceAPI/target/apidocs/index.html...
Generating /home/epicsv4/masterJava/exampleJava/serviceAPI/target/apidocs/help-doc.html...
[INFO] Building jar: /home/epicsv4/masterJava/exampleJava/serviceAPI/target/epics-examples-serviceAPI-4.3.0-SNAPSHOT-javadoc.jar
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO] 
[INFO] exampleJava ........................................ SUCCESS [  1.573 s]
[INFO] exampleJava - arrayPerformance ..................... SUCCESS [  2.922 s]
[INFO] exampleJava - database ............................. SUCCESS [  1.395 s]
[INFO] exampleJava - exampleClient ........................ SUCCESS [  1.677 s]
[INFO] exampleJava - exampleLink .......................... SUCCESS [  1.541 s]
[INFO] exampleJava - helloPutGet .......................... SUCCESS [  1.152 s]
[INFO] exampleJava - helloRPC ............................. SUCCESS [  1.201 s]
[INFO] exampleJava - powerSupply .......................... SUCCESS [  1.232 s]
[INFO] exampleJava - pvDatabaseRPC ........................ SUCCESS [  1.568 s]
[INFO] exampleJava - serviceAPI ........................... SUCCESS [  1.108 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 16.983 s
[INFO] Finished at: 2018-08-14T15:15:39-04:00
[INFO] Final Memory: 30M/644M
[INFO] ------------------------------------------------------------------------

So what am I expected to do in response to

it seems like the javadoc issues are now with https://github.com/epics-base/exampleJava I will request Marty to have a look at it

@shroffk
Copy link
Contributor Author

shroffk commented Aug 20, 2018

@mrkraimer I wanted you to look at the code changes here and let us know if you see any glaring problems.
If you look at the pvData code you will notice the use of the PVNumberArray interface instead of the PVScalarArray...the PVNumberArray does the transformation, gives access to the array elements without requiring copy, etc...

@georgweiss
Can we merge this and open a new issue ticket about the VImageType and its use of ordinals. We will not release epicsCoreJava until we resolve this.

@mrkraimer
Copy link
Contributor

Look OK to me.
Go ahead and merge.

@georgweiss
Copy link
Contributor

georgweiss commented Aug 21, 2018 via email

@shroffk shroffk merged commit 72bca67 into master Aug 23, 2018
@shroffk shroffk deleted the updated_pvdata branch September 27, 2018 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants